Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[8472] Migrate project point to GeoDjango and add a new serializer mixin to convert between gis and geojson #1765

Merged
merged 2 commits into from
Feb 18, 2025
Merged

[8472] Migrate project point to GeoDjango and add a new serializer mixin to convert between gis and geojson #1765

merged 2 commits into from
Feb 18, 2025

Conversation

goapunk
Copy link
Contributor

@goapunk goapunk commented Jan 28, 2025 •
edited
Loading

This is a first attempt to move geographical locations from being stored as plain json/geojson to GeoDjango. This will allow us to filter for locations on a database level.

Tasks

  • PR name contains story or task reference
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@goapunk goapunk force-pushed the jd-2025-01-custom-geometry-field branch 8 times, most recently from 3599699 to f89db5d Compare January 30, 2025 17:33
@goapunk goapunk requested review from m4ra and partizipation January 30, 2025 18:00
@goapunk goapunk mentioned this pull request Jan 30, 2025
Merged
5 tasks
@m4ra
Copy link
Contributor

m4ra commented Feb 10, 2025

@goapunk what's your concern at this point?

@goapunk
Copy link
Contributor Author

goapunk commented Feb 11, 2025

@goapunk what's your concern at this point?

@m4ra just that it's a rather big change. And we need to keep in mind this only migrates the points for projects. Points for ideas/proposals and other geometries (e.g. polygon) will stay as json fields.

m4ra
m4ra requested changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this surgical operation :)
It needs a good docs, with payload example and an explanation on why we keep both paradigms, and maybe in the future all map based modules should migrate to django gis

adhocracy4/projects/migrations/0049_project_geos_point.py
name="point",
),
migrations.AddField(
model_name="project",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think once data has migrated wth migrate_project_point_field and old point filed is removed you can just rename the field from geos_point back to point, but safer in a new migration. And no need for migrate_project_geos_point_field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sadly the RenameField migration doesn't work. It just gives a cryptic error. I assume it's because the old field is a normal sqlite field and the new one is a spatialite gis field, but that's more of a guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have a look again just in case I missed something

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, just leave it as it is then

adhocracy4/projects/migrations/0049_project_geos_point.py
+ ": "
+ str(geojson_point)
)
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this log warning be showing in sentry? otherwise i am afraid we will forget to check when deloying to prod/stage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it won't show up in sentry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd locally test the migration with the prod db though

@m4ra
Copy link
Contributor

m4ra commented Feb 11, 2025

@partizipation this PR, introducing django.gis for storing geopoints in the database

@goapunk goapunk force-pushed the jd-2025-01-custom-geometry-field branch from f89db5d to 5cbff12 Compare February 12, 2025 12:21
@goapunk goapunk requested a review from m4ra February 12, 2025 12:21
@goapunk
Copy link
Contributor Author

goapunk commented Feb 12, 2025

@m4ra landing this means we have to enable postgis on all our platforms using a4, this also means we need changes in the admin repo

@m4ra
Copy link
Contributor

m4ra commented Feb 17, 2025

@goapunk we start with mB then in admin, and @P4rcev4l enable postgis for the other projects in the admin one by one?

@m4ra landing this means we have to enable postgis on all our platforms using a4, this also means we need changes in the admin repo

@goapunk
Copy link
Contributor Author

goapunk commented Feb 17, 2025

t

@goapunk we start with mB then in admin, and @P4rcev4l enable postgis for the other projects in the admin one by one?

@m4ra landing this means we have to enable postgis on all our platforms using a4, this also means we need changes in the admin repo

yes sounds good

@goapunk goapunk changed the title WIP: [8472] Migrate project point to GeoDjango and add a new serializer mixin to convert between gis and geojson [8472] Migrate project point to GeoDjango and add a new serializer mixin to convert between gis and geojson Feb 17, 2025
m4ra
m4ra reviewed Feb 17, 2025
View reviewed changes
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goapunk thanks, LGTM. I leave it to you to merge it once the admin repo is ready. I would still think worthy to add a docs entry with this new feature/implementation.

@goapunk goapunk force-pushed the jd-2025-01-custom-geometry-field branch 2 times, most recently from 0435724 to 4b1ee57 Compare February 17, 2025 16:39
@goapunk
Copy link
Contributor Author

goapunk commented Feb 17, 2025

@m4ra I added some (mostly autogenerated) docs, what do you think?

@goapunk
maps: add custom PointSerializerMixin which allows to save geojson
0ee057a 
points as GeoDjango to the database for better filtering capabilites and
serializes them as geojson features.

- add PointInPolygon validator which validates that a given point is in
  the provided polygon.
@goapunk
add .vscode/ to gitignore
7139c8b
@goapunk goapunk force-pushed the jd-2025-01-custom-geometry-field branch from 4b1ee57 to 7139c8b Compare February 17, 2025 16:55
@m4ra m4ra merged commit bd82532 into main Feb 18, 2025
2 checks passed
@m4ra m4ra deleted the jd-2025-01-custom-geometry-field branch February 18, 2025 11:36
@goapunk goapunk mentioned this pull request Feb 19, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m4ra m4ra m4ra approved these changes

@partizipation partizipation Awaiting requested review from partizipation

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@goapunk @m4ra